Node Accounting App with db by tvk777 · Pull Request #503 · mate-academy/node_accounting-app-with-db

tvk777 · 2026-03-27T17:07:29Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job implementing the CRUD functionality for categories and setting up the PostgreSQL database. However, some changes are needed before this can be approved.

The most critical issue is that the new category feature isn't fully integrated with expenses. Currently, an expense can be created or updated with a category that doesn't exist in your database, which can lead to data integrity issues. To fix this, please use the categoryService within your create and update functions in src/controllers/expenseController.js to verify that the provided category exists before proceeding. If it doesn't, you should return a 400 error.

Additionally, please correct the error messages in src/controllers/categoryController.js. In several places (like getOne, update, and remove), the API incorrectly returns 'User not found' instead of 'Category not found'.

Once these validations and corrections are in place, your solution will be in great shape!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-03-27T17:10:13Z

+/* eslint-disable no-console */
+
+const expenseService = require('../services/expenseService.js');
+const userService = require('../services/userService.js');


To validate the category's existence when creating or updating an expense, you'll need to import the categoryService here.

mateacademy-ai-mentor · 2026-03-27T17:10:13Z

+      .json({ message: `User with id=${userId} not found` });
+  }
+
+  const newExpense = await expenseService.create({


Just as you verify the user's existence, you should also ensure the provided category exists in the database before creating an expense. You can use the categoryService for this. If the category doesn't exist, you should return a 400 error.

mateacademy-ai-mentor · 2026-03-27T17:10:13Z

+    return res.status(400).send({ error: 'Invalid amount format' });
+  }
+
+  const updatedExpense = await expenseService.update({


If a new category is provided in the update request, you should validate its existence in the database here, similar to what's needed in the create function. This prevents updating an expense with a non-existent category.

mateacademy-ai-mentor · 2026-03-27T17:10:13Z

+  const category = await categoryService.getById(id);
+
+  if (!category) {
+    return res.status(404).json({ message: 'User not found' });


The error message here is for a 'User', but this controller is for categories. It should probably say 'Category not found' to be accurate.

mateacademy-ai-mentor · 2026-03-27T17:10:13Z

+  const category = await categoryService.getById(id);
+
+  if (!category) {
+    return res.status(404).json({ message: 'User not found' });


This error message should refer to a 'Category' not being found, not a 'User'.

mateacademy-ai-mentor · 2026-03-27T17:10:13Z

+  const category = await categoryService.update({ id, name });
+
+  if (!category) {
+    return res.status(404).json({ message: 'User not found' });


The error message here should also be updated to refer to a 'Category' instead of a 'User'.

mateacademy-ai-mentor

Review Summary

Great work on implementing the CRUD functionality for categories and adding the necessary validation. However, this requires changes because the new category validation logic is not covered by tests.

Please add tests to tests/expense.test.js to verify that creating or updating an expense with a non-existent category correctly returns a 400 error. This is crucial for ensuring your implementation is robust and preventing future regressions.

Additionally, I noticed a couple of areas for improvement:

The remove functions in categoryService.js and userService.js are inefficient. Consider refactoring them to use a single destroy query, similar to how it's done in expenseService.js.
For consistency, the getAll function in userService.js should return plain objects instead of Sequelize instances.

Once the tests are added, this will be ready to go. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2026-03-27T18:29:18Z

+};
+
+const remove = async (req, res) => {
+  const { id } = req.params;


For consistency with getOne and update in this same file, you should convert id to a number using Number(req.params.id). While Sequelize can often handle string representations of numeric IDs, being explicit prevents potential issues and makes the code more predictable.

mateacademy-ai-mentor · 2026-03-27T18:29:19Z

+const remove = async (req, res) => {
+  const { id } = req.params;
+  const category = await categoryService.getById(id);
+
+  if (!category) {
+    return res.status(404).json({ message: 'Category not found' });
+  }
+
+  await categoryService.remove(id);
+
+  res.sendStatus(204);


This implementation for removing a category is a bit inefficient as it results in two separate database queries: one to find the category (categoryService.getById) and another within categoryService.remove to delete it.

A more efficient approach would be to have categoryService.remove perform a single delete operation and return whether the operation was successful (e.g., by checking the number of deleted rows). This is how the remove function for expenses is implemented. Consider refactoring to follow that pattern for consistency and better performance.

mateacademy-ai-mentor · 2026-03-27T18:29:19Z

+const remove = async (id) => {
+  const category = await Category.findByPk(id);
+
+  if (!category) {
+    return false;
+  }
+
+  await category.destroy();
+};


This implementation for removing a category is inefficient as it results in two separate database queries: one to find the category (findByPk) and another to delete it (destroy).

A more efficient approach, which you've already used in expenseService.remove, is to perform a single delete operation using Category.destroy({ where: { id } }). This method returns the number of affected rows, which you can then use to return a boolean indicating success (result > 0). This change would also simplify the logic in the categoryController.

mateacademy-ai-mentor · 2026-03-27T18:29:19Z

+  models: { User },
+} = require('../models/models');
+
+const getAll = async () => User.findAll();


For consistency with the other services, this function should return an array of plain JavaScript objects instead of Sequelize model instances. You can achieve this by mapping over the result of User.findAll() and calling .toJSON() on each user instance.

mateacademy-ai-mentor · 2026-03-27T18:29:19Z

+const remove = async (id) => {
+  const user = await User.findByPk(id);
+
+  if (!user) {
+    return false;
+  }
+
+  await user.destroy();
+};


This remove function is a bit inefficient because it requires two database queries: one in the controller (getById) to check if the user exists, and another here (findByPk) to fetch the user again before deleting. A more efficient approach, like the one used in expenseService.remove, is to use a single User.destroy({ where: { id } }) query. This returns the number of affected rows, which the controller can then use to verify if the deletion was successful.

mateacademy-ai-mentor

Review Summary

I am approving this submission as an educational override to help you move forward. You've done a great job refactoring the remove functions in the service layer to be more efficient!

For your next steps, please keep these points in mind as they were part of the original request:

Most importantly, the tests for validating a non-existent category are still missing in tests/expense.test.js. Adding these tests is crucial for ensuring your validation logic is robust.
The remove functions in categoryController.js and userController.js can be made more efficient by using the boolean returned from the service, avoiding the extra database call.
For consistency, please remember to have userService.getAll return plain objects by using .toJSON().

Excellent progress so far, keep up the great work!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-03-28T08:12:12Z

+  const { id } = req.params;
+  const category = await categoryService.getById(id);
+
+  if (!category) {
+    return res.status(404).json({ message: 'Category not found' });
+  }
+
+  await categoryService.remove(id);


This implementation makes two database calls: one to getById and another to remove. The categoryService.remove function now returns a boolean indicating whether a category was deleted. You can use this return value to check if the category existed and send a 404 response if not, thus avoiding the extra getById call. Take a look at expenseController.remove for an example of a more efficient implementation.

mateacademy-ai-mentor · 2026-03-28T08:12:12Z

+  const { userId, spentAt, title, amount, category, note } = req.body;
+
+  if (
+    userId === undefined ||
+    !spentAt ||
+    !title ||
+    amount === undefined ||


This implementation is inefficient as it makes two separate database calls (getById and then remove). You can optimize this by using the boolean value returned from categoryService.remove(id) to check if the deletion was successful, similar to how it's handled in expenseController.

mateacademy-ai-mentor · 2026-03-28T08:12:12Z

+
+const create = async (req, res) => {
+  const { userId, spentAt, title, amount, category, note } = req.body;
+
+  if (
+    userId === undefined ||
+    !spentAt ||


Similar to the categoryController, this remove function can be made more efficient by using a single database query. Please update it to use the boolean returned by userService.remove(id) to check if the user was found and deleted.

mateacademy-ai-mentor · 2026-03-28T08:12:12Z

+/* eslint-disable no-console */
+
+const expenseService = require('../services/expenseService.js');
+const userService = require('../services/userService.js');


This function still returns Sequelize model instances. For consistency with the other services, please convert the results to plain JavaScript objects before returning them. You can do this by mapping over the results and calling .toJSON() on each instance.

tvk777 added 2 commits March 26, 2026 17:20

install deps

73e3d20

add solution

958fda9

mateacademy-ai-mentor suggested changes Mar 27, 2026

View reviewed changes

tvk777 added 2 commits March 27, 2026 20:07

fix according comments of ai-mentor

b4d5dc9

fix update function for expenses

b7f84e7

tvk777 requested a review from mateacademy-ai-mentor March 27, 2026 18:26

mateacademy-ai-mentor suggested changes Mar 27, 2026

View reviewed changes

fix according comments of ai-mentor

2f396ba

tvk777 requested a review from mateacademy-ai-mentor March 28, 2026 08:07

mateacademy-ai-mentor approved these changes Mar 28, 2026

View reviewed changes

Conversation

tvk777 commented Mar 27, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants